fix: Fix setting status of delete button in multiple files widget - #1443
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the delete button enable/disable logic in the multiple files selection widget to depend on the current selection and safely handle deletion and reset states. State diagram for delete button enable/disable logic in multiple files widgetstateDiagram-v2
[*] --> Disabled
Disabled --> Enabled: file_chosen
note right of Enabled
condition: selected_files.currentRow() != -1
end note
Enabled --> Disabled: delete_element
note right of Disabled
condition: selected_files.count() == 0
end note
Enabled --> Disabled: clean
Disabled --> Disabled: clean
Enabled --> Enabled: file_chosen
note right of Enabled
condition: selected_files.currentRow() != -1
end note
Disabled --> Disabled: file_chosen
note right of Disabled
condition: selected_files.currentRow() == -1
end note
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe file selection widget now deletes all selected files and updates the delete-button state when files are selected or cleared. A regression test covers deletion of the only file. Codecov GitHub Checks annotations are enabled. ChangesFile selection controls
Codecov checks
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change keeps the multiple-file widget’s delete action synchronized with the current selection and file list, with focused test coverage for deleting the final file. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title accurately identifies the main change: fixing delete button behavior in the multiple files widget. It is specific enough for repository history, although the repeated use of "Fix" is slightly redundant. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider centralizing the delete button enable/disable logic into a small helper (e.g.
_update_delete_button_state()) and calling it fromfile_chosen,delete_element, andcleanto keep the behavior consistent and easier to maintain. - For clarity and consistency with
setEnabled(...)usage elsewhere, you may want to replaceself.delete_button.setDisabled(True)incleanwithself.delete_button.setEnabled(False).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the delete button enable/disable logic into a small helper (e.g. `_update_delete_button_state()`) and calling it from `file_chosen`, `delete_element`, and `clean` to keep the behavior consistent and easier to maintain.
- For clarity and consistency with `setEnabled(...)` usage elsewhere, you may want to replace `self.delete_button.setDisabled(True)` in `clean` with `self.delete_button.setEnabled(False)`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package/PartSeg/common_gui/select_multiple_files.py`:
- Around line 231-236: Update delete_button enablement and delete_element to
require an actually selected item rather than relying on currentRow(). Use
selectedItems(), currentItem() with isSelected(), or equivalent selection
validation before enabling deletion and calling takeItem, while preserving the
existing no-item early return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c2685fc-8a5d-4b86-99c1-527fa0712ff8
📒 Files selected for processing (1)
package/PartSeg/common_gui/select_multiple_files.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1443 +/- ##
========================================
Coverage 92.60% 92.61%
========================================
Files 211 211
Lines 33236 33248 +12
========================================
+ Hits 30779 30792 +13
+ Misses 2457 2456 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package/PartSeg/common_gui/select_multiple_files.py`:
- Around line 233-239: Update delete_element to remove each deleted FileListItem
from selected_files using takeItem with the item’s current row before or
alongside removing its path from files_to_proceed. Set delete_button enabled
state from the remaining widget selection/count after deletion, and extend
test_delete_element to assert the widget count is updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0bcc6364-173e-47cd-b2f0-97f8683b504e
📒 Files selected for processing (2)
codecov.ymlpackage/PartSeg/common_gui/select_multiple_files.py
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix PARTSEG-10D
Summary by Sourcery
Ensure the multiple-file widget keeps its delete action state synchronized with the current selection and file list.
Bug Fixes:
CI:
Tests:
Summary by CodeRabbit